Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infra: Use an option instead of argument in banner directives #3765

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Apr 28, 2024

(context for PEP authors: This PR will not change how a specific PEP is presented or how it would be rendered -- if you're not interested in PEP infra changes, please feel welcome to unsubscribe)

This is an "infra" change, tweaking the specific reST syntax structures being used in the directives used to provide document-wide deprecations or other links. See also #2719, which tracks the wider effort around this.

For the problem that motivated this change, see #3682 and #3756. See the commit message for the details of what the problem's cause was and how this change deals with that.


📚 Documentation preview 📚: https://pep-previews--3765.org.readthedocs.build/

@pradyunsg pradyunsg added the infra Core infrastructure for building and rendering PEPs label Apr 28, 2024
@pradyunsg
Copy link
Member Author

pradyunsg commented Apr 28, 2024

Sphinx presents a clear error message if you have any content without whitespace after these banner directive's options:

With this change added in...

--- a/peps/pep-0560.rst
+++ b/peps/pep-0560.rst
@@ -12,6 +12,7 @@ Resolution: https://mail.python.org/pipermail/python-dev/2017-December/151038.ht
 .. canonical-doc::
    :related: :external+python:meth:`object.__class_getitem__` and
              :external+python:meth:`object.__mro_entries__`
+   Content
 
 Abstract
 ========

Running a build results in:

Sphinx parallel build error:
docutils.utils.SystemMessage: /Users/pradyunsg/Developer/github/peps/peps/pep-0560.rst:12: (ERROR/3) Error in "canonical-doc" directive:
invalid option block.

.. canonical-doc::
   :related: :external+python:meth:`object.__class_getitem__` and
             :external+python:meth:`object.__mro_entries__`
   Content
Sphinx exited with exit code: 2

While this behaviour is mentioned in the commit message, I figured it'd be useful to have this error message mentioned here to provide additional context.

This is a good place to also mention a caveat(?) with this approach: If there's an additional space (i.e. indentation of Content is greater than that of :related:, it'll be considered a part of the option's value). I don't think it's an equivalent failure mode to what we have right now but also... I don't see any way to avoid this behaviour from docutils/reST.

@hugovk hugovk changed the title Use an option instead of argument in banner directives Infra: Use an option instead of argument in banner directives Apr 28, 2024
When a reST directive has `has_content` set to `True`, optional
arguments and `final_argument_whitespace` set to True, the presence of a
newline at the start of the directive is significant in communicating
whether specific markup is treated as the argument or content.

    .. banner-1:: This is an argument
       This is still the argument.

       This is content.

    .. banner-2::
       This is still the argument.

       This is content.

    .. banner-3::

       This is content.

       This is more content.

In the above example, `banner-2` and `banner-3` are very similar and
only different in the presence of a newline. This is a subtle failure
mode where written content can end up being silently ignored by the reST
directive due to how arguments vs contents are parsed.

Instead of accommodating for this behaviour by adding additional
complexity to the directive being used, this change stops trying to mix
multiline arguments and content in a single directive by using explicit
options instead. This requires more verbosity but also eliminates this
failure mode entirely.

    .. banner-1::
       :option: This is the option.
                This is still the option.

    .. banner-2::
       This is content.

       This is still content.

    .. banner-3::

       This is content.

       This is still content.

With this change, presence of a newline at the start of the directive is
not determining if specific markup is treated as the argument or
content. This is instead communicated by the presence of the option
syntax which is more explicit and presents proper errors when the syntax
is incorrect.
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sensible.

A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Core infrastructure for building and rendering PEPs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants